Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sched: add a registration-based scheduler to argobots #373

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

sched: add a registration-based scheduler to argobots #373

wants to merge 1 commit into from
+370 −1

Conversation

epmikida
Copy link

Pull Request Description

This adds a basic registration-based scheduler option to Argobots, which allows users to
associate a scheduling policy with each pool that is passed to the scheduler which determines
how the scheduler will select which pool to pop from at each iteration of the scheduler loop.

Checklist

  • Reference appropriate issues (with "Fixes" or "See" as appropriate)
  • Commits are self-contained and do not do two things at once
  • Commit message is of the form: module: short description and follows good practice
  • Passes whitespace checkers

@epmikida
sched: Add a registration-based scheduler to Argobots
aed832e 
This scheduler allows users to associate simple policies with
each pool that dictate how the scheduler selects the pool to pop
from at each iteration of the scheduler loop.
@shintaro-iwasaki
Copy link
Collaborator

Thank you for your contribution, @epmikida!

Before diving into the detailed code review, can I ask you the following?

  1. Please add at least one small test in test/basic/ (in a different commit).
  2. Please fix all the whitespace/compilation problems. The CI tests will be run automatically whenever you push changes
  3. Please rebase the branch to the latest main branch (GitHub says This branch is out-of-date with the base branch)
  4. Please explain a typical use case of the new scheduler. It'd be great if you could give a small code snippet.
  5. Similar to 4: please explain merits of this new scheduler over the existing user-defined scheduler from a viewpoint of performance, abstraction, and/or functionality.
    • Particularly, do we need a new function for this new scheduler type? Isn't it another "predefined" scheduler (see ABT_sched_predef)?

(Please let me note that I am not opposing to the idea, and I believe I know the idea and like it!)

@epmikida
Copy link
Author

Thank you for your contribution, @epmikida!

Before diving into the detailed code review, can I ask you the following?

  1. Please add at least one small test in test/basic/ (in a different commit).

  2. Please fix all the whitespace/compilation problems. The CI tests will be run automatically whenever you push changes

  3. Please rebase the branch to the latest main branch (GitHub says This branch is out-of-date with the base branch)

  4. Please explain a typical use case of the new scheduler. It'd be great if you could give a small code snippet.

  5. Similar to 4: please explain merits of this new scheduler over the existing user-defined scheduler from a viewpoint of performance, abstraction, and/or functionality.

    • Particularly, do we need a new function for this new scheduler type? Isn't it another "predefined" scheduler (see ABT_sched_predef)?

(Please let me note that I am not opposing to the idea, and I believe I know the idea and like it!)

Yes, I'll get working on these changes right away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
add API enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@epmikida @shintaro-iwasaki